Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some Bitswap Refactoring #140

Closed
wants to merge 4 commits into from

Conversation

llSourcell
Copy link
Contributor

This PR Includes

-making a clarifying comment on why we are currently using AlwaysSendtoPeer as a BS strategy
-re-ordering functions so that types are above functions (looks cleaner)
-There was a TODO regarding using float64 in the strategy that could've resulted in a lock, avoiding the lock
-We were using nice as a word for how we view other peers, changed that to 'trusted'. We want to bitswap to be an auction that involves trust-the strategy should vary based on how trustworthy we are, and how much we trust the peer.

func standardStrategy(l *ledger) bool {
return rand.Float64() <= probabilitySend(l.Accounting.Value())
r := rand.New(rand.NewSource(1234))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the rand object we instantiate should be stored somewhere, possibily in the ledger. and should be constructed when the ledger is created.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the seed should be seeded by time, (or a random number from crypto/rand)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@llSourcell ... why did you introduce a constant seed? This is wrong. rand.Float64 will draw from the default source, which we can initialize with a random seed using rand.Seed.

@jbenet
Copy link
Member

jbenet commented Oct 4, 2014

@llSourcell this PR introduces two changes which are incorrect.

The rest is just moving code around in a single file, which is not actually useful. Go goes to great lengths to make code position in files a non-issue. (Specifying a canonical style + gofmt for example). Moving code around in the same file might help readability a bit, but frankly, most Go codebases (including the stdlib) are a mess in terms of code positioning. And with godoc and good editors, you can find the code in question quickly anyway.

Sorry @llSourcell, but this kind of PR is another that has mainly wasted your and our time. :( I really appreciate your willingness and effort to help IPFS. But please familiarize yourself more with Go and the systems at hand (like rand here). And you should connect with me directly on irc before spending your time on things that will end up not correct or not important.

👍 thanks, and sorry!

@jbenet jbenet closed this Oct 4, 2014
@aschmahmann aschmahmann mentioned this pull request Feb 18, 2021
73 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants